Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Image Controls #1617

Merged
merged 8 commits into from
Jan 18, 2025
Merged

Image Controls #1617

merged 8 commits into from
Jan 18, 2025

Conversation

EricBAndrews
Copy link
Member

@EricBAndrews EricBAndrews commented Jan 17, 2025 •
edited
Loading

Checklist

  • I have read CONTRIBUTING.md
  • I have described what this PR contains
  • If this PR alters the UI, I have attached pictures/videos
  • This PR addresses one or more open issues that were assigned to me:
    - closes #issue
    - progress towards #issue

Pull Request Information

Replaces the image viewer context menu with a control layer.

Intended behavior:

  • Control layer is shown when viewer first opens
  • For non-animated media: control layer appears/disappears on tap
  • For animated media:
    • When playing: control layer appears/disappears on tap outside of media's frame (i.e., in the black margins)
    • When not playing: control layer appears/disappears on tap anywhere but the play button

This PR also removes the ProgressView from AnimationControlLayer. Media loading is almost always fast enough that the ProgressView is just visual clutter, and in rare cases a gif advertises itself as "animated" but isn't and leaves the ProgressView spinning on top of it (I don't have a repro example handy, but I have seen it a couple times).

@EricBAndrews EricBAndrews requested a review from a team as a code owner January 17, 2025 22:13
@EricBAndrews EricBAndrews requested review from Sjmarf and removed request for a team January 17, 2025 22:13
@EricBAndrews EricBAndrews marked this pull request as draft January 18, 2025 05:20
@EricBAndrews EricBAndrews marked this pull request as ready for review January 18, 2025 15:48
Sjmarf
Sjmarf requested changes Jan 18, 2025
View reviewed changes
Copy link
Member

@Sjmarf Sjmarf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 😄 A couple of design questions

Mlem/App/Views/Pages/ImageViewer.swift Outdated Show resolved Hide resolved
Mlem/App/Views/Pages/ImageViewer.swift Outdated Show resolved Hide resolved
@EricBAndrews
Copy link
Member Author

EricBAndrews commented Jan 18, 2025 •
edited
Loading

Per Slack, controls now (should) work as follows:

  • On single tap gesture: hide/show with opacity animation
  • On zoom in: hide with opacity + slide animation the moment the zoom starts
    • If the user has tapped to show controls while the image is already zoomed, controls should not auto-hide (unless zoom scale resets to 1.0 and the user zooms back in from there)
  • On zoom out: show with opacity + slide animation
    • On double tap gesture, this happens immediately
    • On pinch gesture, this happens when zoom scale hits 1.0; this prevents the controls from popping in and out if the user is pinching around an image
  • On interactive dismiss: slide behavior unchanged, fade now matches slide-and-fade (i.e., fades to 0 at maxControlOffset)

I've also refactored the control layer view code into ImageViewer+Views.

@EricBAndrews EricBAndrews requested a review from Sjmarf January 18, 2025 17:57
Sjmarf
Sjmarf approved these changes Jan 18, 2025
View reviewed changes
Copy link
Member

@Sjmarf Sjmarf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, works beautifully 😄

@EricBAndrews EricBAndrews merged commit 6fce5b1 into dev Jan 18, 2025
2 checks passed
@EricBAndrews EricBAndrews deleted the eric/image-controls branch January 18, 2025 19:55
@EricBAndrews EricBAndrews restored the eric/image-controls branch January 18, 2025 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sjmarf Sjmarf Sjmarf approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@EricBAndrews @Sjmarf